added more clarification #1395
added more clarification #1395
Conversation
|
@ConorOkus please is this in line? |
Codecov Report
@@ Coverage Diff @@
## main #1395 +/- ##
==========================================
- Coverage 90.76% 90.76% -0.01%
==========================================
Files 73 73
Lines 41195 41195
Branches 41195 41195
==========================================
- Hits 37392 37391 -1
- Misses 3803 3804 +1
Continue to review full report at Codecov.
|
| @@ -315,11 +315,12 @@ pub trait Watch<ChannelSigner: Sign> { | |||
| /// [BIP 157]: https://github.com/bitcoin/bips/blob/master/bip-0157.mediawiki | |||
| /// [BIP 158]: https://github.com/bitcoin/bips/blob/master/bip-0158.mediawiki | |||
| pub trait Filter { | |||
| /// Registers interest in a transaction with `txid` and having an output with `script_pubkey` as | |||
| /// Registers interest in funding transactions to inform LDK that a channel | |||
| /// Funding transaction is transaction with `txid` and having an output with `script_pubkey` as | |||
There was a problem hiding this comment.
I don't think we want to narrow the definition of this function to only funding transaction(s). Yes, it's true today, but specifying it in the docs and committing to it I don't think we want to do.
There was a problem hiding this comment.
What would be a better description that clarifies and differentiates what it does with register_output
There was a problem hiding this comment.
The key difference here is one looks at a given transaction being broadcasted, one looks for spends of a given output being broadcasted.
There was a problem hiding this comment.
@TheBlueMatt, should I go with this;
/// Registers interest in transactions to inform LDK that a channel.
/// Transactions with txid and having an output with script_pubkey as
/// a spending condition.
There was a problem hiding this comment.
I dont think these functions should have any reference to what kind of transaction it is (channel/close/open/etc).
There was a problem hiding this comment.
So I should leave them at what they were?
There was a problem hiding this comment.
I mean I do think they need to be clarified, because we've seen a good chunk of confusion on them, but it remains entirely unclear to me exactly how they should be clarified :/
| /// a spending condition. | ||
| fn register_tx(&self, txid: &Txid, script_pubkey: &Script); | ||
|
|
||
| /// Registers interest in spends of a transaction output. | ||
| /// Registers interest in spends of a force close transaction. |
There was a problem hiding this comment.
This isn't just about force-closure transactions - we also use it to detect normal closures, etc.
There was a problem hiding this comment.
@TheBlueMatt is this a better description; Registers interest in spends of a closing transaction.
There was a problem hiding this comment.
Its not "spends of a closing transaction", though, its "spends of a given outpoint", ie any time a given output is spent we care.
|
Any interest in continuing to work on this, @gabbyprecious? Sorry it ended up being a bit unclear what the docs should say. |
|
@TheBlueMatt yes, is there any clarity on what it should say. Will also be checking other issues |
|
Sorry for the late reply, we had the lightning spec summit last week. I don't have perfect clarity - I've seen a lot of user confusion on this API, so I think we do need to clarify, but its kinda unclear to me how to do so (without changing the definition/API contract here). Do you find the meaning of |
|
Any desire to keep working on this @gabbyprecious? |
|
Yes, if there's more clarity on it. |
|
Sorry for the delay in responding. Maybe we just include in the docs a specific contrast between the two methods. Ie in one of them write exactly how it differs from the other, noting that one is about spends and the other is about transactions themselves. |
|
Closing in favor of #3036 |
No description provided.